fix: extend bad_spec to cover data-loss findings within story domain by gabadi · Pull Request #2139 · bmad-code-org/BMAD-METHOD

gabadi · 2026-03-27T03:58:28Z

What

Extends the bad_spec classification in quick-dev step-04-review to cover findings where data is silently dropped or never reaches its destination within the story's domain — even if the code predates the diff.

Why

In a real-world quick-dev run, an edge case hunter correctly identified a missing field mapping (data silently dropped at a domain boundary). The finding was misclassified as defer because the diff didn't touch that line. The bug shipped and was only caught by human review.

Fixes #2138

How

Added one sentence to the bad_spec definition in step-04-review.md: data-loss findings within the story's domain qualify as bad_spec, not defer
No changes to defer definition — the gate lives where the reviewer looks when deciding what qualifies as bad_spec

Testing

Validated through adversarial review and edge case analysis of the proposed change. The rule correctly reclassifies the original finding (missing learningPopulation on CreditCheck builder) as bad_spec while leaving genuinely out-of-scope pre-existing issues as defer.

…n story domain

augmentcode · 2026-03-27T03:59:32Z

🤖 Augment PR Summary

Summary: Extends the quick-dev step-04-review guidance so in-scope data-loss findings are treated as bad_spec rather than defer.
Why: Prevents silently-dropped data issues at story-domain boundaries from being misclassified and shipped when the underlying code predates the current diff.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-03-27T03:59:33Z

src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md

 2. Classify each finding. The first three categories are **this story's problem** — caused or exposed by the current change. The last two are **not this story's problem**.
   - **intent_gap** — caused by the change; cannot be resolved from the spec because the captured intent is incomplete. Do not infer intent unless there is exactly one possible reading.
-   - **bad_spec** — caused by the change, including direct deviations from spec. The spec should have been clear enough to prevent it. When in doubt between bad_spec and patch, prefer bad_spec — a spec-level fix is more likely to produce coherent code.
+   - **bad_spec** — caused by the change, including direct deviations from spec. The spec should have been clear enough to prevent it. When in doubt between bad_spec and patch, prefer bad_spec — a spec-level fix is more likely to produce coherent code. Also applies when a finding reveals data silently dropped or never reaching its destination within the story's domain — even if the code predates the diff.


This bad_spec definition still starts with “caused by the change” but then says it applies even if the code predates the diff, which could confuse reviewers about when pre-existing issues should be treated as this story’s problem. Consider clarifying what qualifies as “within the story’s domain” (and whether being merely in-scope vs. caused/exposed-by-change is the deciding factor) to keep classifications consistent.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

coderabbitai · 2026-03-27T04:00:29Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3357d1cf-91d1-4439-afd9-1dc4148b7b65

📥 Commits

Reviewing files that changed from the base of the PR and between 1040c3c and 229c1c5.

📒 Files selected for processing (1)

src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md

📝 Walkthrough

Walkthrough

Documentation update to the quick-dev review classification process. The bad_spec category definition is expanded to include findings where data is silently dropped or fails to reach its intended destination within the story's domain, regardless of whether the code predates the change.

Changes

Cohort / File(s)	Summary
Quick-Dev Review Documentation `src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md`	Expanded `bad_spec` classification to cover data-loss findings (silent drops or missed destinations) within the story's domain, preventing misclassification as pre-existing findings when they fall under initiative scope.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

BMAD-METHOD#2007: Introduces or relates to the five-category review-finding classification system (including bad_spec) that this PR refines.

Suggested reviewers

bmadcode

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: extending bad_spec to cover data-loss findings within the story domain, matching the actual modification to the review classification definition.
Description check	✅ Passed	The description clearly explains what, why, and how the change addresses the problem, with relevant context about the real-world scenario that motivated the fix.
Linked Issues check	✅ Passed	The PR fully addresses issue `#2138` by extending the bad_spec definition to include data-loss findings within the story's domain, preventing misclassification as defer.
Out of Scope Changes check	✅ Passed	The changes are narrowly scoped to the bad_spec definition in step-04-review.md, directly addressing issue `#2138` without introducing unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…' language

alexeyv · 2026-03-29T21:52:12Z

@adambiggs Thanks for the PR. Way I see it, you discovered a fundamental failure mode, but the PR is a patch for one of the symptoms thereof. It solves your situation that exposed the failure mode, but not the entire failure mode.

So I've tried to generalize the rule to anything that the intent implies but spec doesn't cover, and here is a version I came up with. Can you check that it classifies your data loss finding as bad spec?

Classify each finding by checking categories in strict order. Assign the first that applies.

  This story's problem — within the story's domain:

  - intent_gap — Spec intent is incomplete; finding cannot be resolved from the spec alone. Do not
  infer intent unless there is exactly one possible reading.
  - bad_spec — Spec should have prevented this: either it directly contradicts the spec, or the spec
   omitted what was needed and a patch would be inconsistent without spec guidance. Prefer bad_spec
  over patch when in doubt.
  - patch — Pure implementation detail, trivially fixable in the diff with no spec change needed.

  Not this story's problem:

  - defer — Real issue clearly outside this story's domain, surfaced incidentally. Prefer reject
  when unsure.
  - reject — Noise or not a real issue. Drop silently.

fix: extend bad_spec classification to cover data-loss findings withi…

229c1c5

…n story domain

augmentcode bot reviewed Mar 27, 2026

View reviewed changes

fix: reframe data-loss clause to align with 'caused/exposed by change…

98f8786

…' language

Merge branch 'main' into fix/quick-dev-review-data-loss-classification

b161491

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: extend bad_spec to cover data-loss findings within story domain#2139

fix: extend bad_spec to cover data-loss findings within story domain#2139
gabadi wants to merge 3 commits intobmad-code-org:mainfrom
gabadi:fix/quick-dev-review-data-loss-classification

gabadi commented Mar 27, 2026

Uh oh!

augmentcode bot commented Mar 27, 2026

Uh oh!

augmentcode bot left a comment

Uh oh!

augmentcode bot Mar 27, 2026

Uh oh!

coderabbitai bot commented Mar 27, 2026

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

alexeyv commented Mar 29, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

gabadi commented Mar 27, 2026

What

Why

How

Testing

Uh oh!

augmentcode bot commented Mar 27, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Mar 27, 2026

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

alexeyv commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexeyv commented Mar 29, 2026 •

edited

Loading